Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replaces Webpack/Babel/Mocha with Vite/Vitest #484

Closed
wants to merge 7 commits into from

Conversation

ralexmatthews
Copy link
Contributor

@ralexmatthews ralexmatthews commented Dec 22, 2024

Description

I decided to start with a much smaller problem to tackle, so this only removes Webpack/Babel/Mocha with Vite/Vitest. I chose Vite/Vitest because that it what Easy-UI uses for its builds and tests.

There is a lot of files changed, but all changes are actually pretty minimal. All this does really is:

  • Remove all webpack/babel/mocha dependencies and related files
  • Installs Vite/Vitest and creates their config files
  • Convert the tests to support Vitest
    • changes the way Polly is set up
    • remove all this. references in the tests
    • all cassettes work in place and none needed to change
  • Makes some changes to github workflows
    • Adds a "test-node-compatibility" test that checks importing and using of the library
    • We don't want to just run the test suite because vite/vitest requires node v18+, but can generate output for arbitrarily low node versions

And thats effectively it. All commands still work as is. There are no changes to the source, and the output file is effectively the same. This does mean that it hasn't fixed any of the weird issues with using the project in Next.js and stuff. That can come at a later date since that will likely require breaking changes to fix.

Overall, this removes like half of the project's deps, and with it, the security vulnerabilities in exchange for a modern toolset.

Testing

All unit tests are passing. In added another test/check specifically for checking compatibility with older node versions. It basically just imports the project and tries to create a test shipment. This makes it so we can test compatibility with old node versions while not requiring the toolchain to support old node versions.

const dotenv = require('dotenv');

dotenv.config();

const EasyPostClient = require('../..');

const test = async () => {
  const apiKey = process.env.EASYPOST_TEST_API_KEY;
  const client = new EasyPostClient(apiKey);

  if (!client.baseUrl.includes('easypost.com')) {
    process.exit(1);
  }
};

test();

I also tested it with using actual API calls with a minimal project using node 12 and 22 and saw it working. I also checked node 10 and confirmed that did not work.

Pull Request Type

Please select the option(s) that are relevant to this PR.

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement (fixing a typo, updating readme, renaming a variable name, etc)

@ralexmatthews ralexmatthews requested a review from a team as a code owner December 22, 2024 22:25
@nwithan8
Copy link
Member

The GitHub CI file is missing the Node version matrix for the build test: https://github.com/EasyPost/easypost-node/actions/runs/12458501554/workflow?pr=484#L22

@ralexmatthews
Copy link
Contributor Author

@nwithan8 Yea thats intentional. The project doesn't build on node 12, 14, or 16 anymore. It will still run the built version on those versions though. Thats what the node-compatibility task is doing, building with node 22, then checking that it still works on those versions.

@nwithan8
Copy link
Member

@nwithan8 Yea thats intentional. The project doesn't build on node 12, 14, or 16 anymore. It will still run the built version on those versions though. Thats what the node-compatibility task is doing, building with node 22, then checking that it still works on those versions.

Alright, we'll remove those as required CI checks then

@ralexmatthews ralexmatthews closed this by deleting the head repository Dec 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants